-
Notifications
You must be signed in to change notification settings - Fork 71
Fix default style (svg width/height) & css prop on promotions #3185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 59b9292 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +34 B (0%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
| <Badge variant={Variant.Green} className={badgeStyles}> | ||
| <Icon glyph="Award" /> | ||
| </Badge> | ||
| <Body as="span" {...rest}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we almost move this {...rest} to the outer div?
| & div { | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this specifically for the badge element? If so, wondering if we can add directly to badgeStyles below
.changeset/curvy-lines-tie.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@lg-chat/message': minor | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a backwards-compatible bugfix, this can be marked as a patch https://semver.org/#summary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was thinking this could be considered a minor change due to the addition of className
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would mark it minor if it was net-new, but in this case, it was already added and getting passed to the Body instance instead of the outermost div
| 'Challenge your knowledge by earning the Advanced Schema Design skill! This is a really really really really really really really really really long copy text to test how the component handles multiline text content.', | ||
| ], | ||
| promotionUrl: ['https://learn.mongodb.com/skills'], | ||
| className: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't typically generate story instances for demonstrating className overrides. Although we'll expose the className prop for components, it's typically an exception rather than a promoted feature to keep UI consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair! I thought it'd be useful to keep a content-box test case, but it might be more appropriate to test that on the Badge component anyway
|
Heads up @hschawe: I've updated where the This PR will likely require a rebase off main to account for this change |
✍️ Proposed changes
Fixes default style on Message.Promotion component. The style
box-sizing: border-boxwas missing, causing the promotion badge size to be wrong when implemented on the MongoDB Assistant. On Storybook,border-boxis set by default on all elements under the Story root element, so this issue was not caught previously.I've also made it possible for users to pass in custom styles on the promotions with
className, as it could be a helpful workaround for users if any similar issues arise.I also updated the stories:
classNameprop, passing differentbox-sizingon the promotion container.✅ Checklist
For bug fixes, new features & breaking changes
HTMLElementProps<'div'>is already documented in the readme]pnpm changesetand documented my changes🧪 How to test changes